Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Special handling for IAR is problematic in zephyr #65

Open
wants to merge 1 commit into
base: zephyr
Choose a base branch
from

Conversation

RobinKastberg
Copy link

In zephyr, zephyr defines __packed. This is an IAR keyword. Inside of zephyr, this is not problematic, since zephyr is not IAR aware (yet). But since zephyr includes third-party modules such as mbedtls which are, the redefinition of the __packed keyword causes compilation problems becauise attribute((packed)) and __packed are not equivalent. Since modern IAR defines both __ARM_FEATURE_UNALIGNED and attribute((packed)) I think we can remove this special handling. Modern IAR is required for zephyr anyways.

In zephyr, zephyr defined `__packed`. This is an IAR keyword.
Inside of zephyr, this is not problematic, since zephyr is not IAR aware
(yet). But since zephyr includes third-party modules such as mbedtls
which are, the redefinition of the `__packed` keyword causes compilation
problems becauise attribute((packed)) and __packed are not equivalent.
Since modern IAR defines both __ARM_FEATURE_UNALIGNED and
attribute((packed)) I think we can remove this special handling.
Modern IAR is required for zephyr anyways.

Signed-off-by: Robin Kastberg <[email protected]>
@tomi-font
Copy link
Collaborator

IAR is not supported in Zephyr, yet you are using Zephyr with IAR? What is your use case exactly?

@valeriosetti
Copy link
Collaborator

I don't think this is the right approach. This PR removes a fix that the upstream Mbed TLS repository specifically added for IAR compiler. Besides it's also hard to check if this has any effects on Zephyr because, as you said, IAR it not supported there so there is test for it. On the other hand, Mbed TLS supports and it is tested with IAR compilers so I don't think it's great to remove this piece of code.

I think the correct approach is to wait for Zephyr to enable support for the IAR compiler, then this issue should be handled automatically (I think that __packed will be renamed to something else, but this is only my guessing).

@RobinKastberg
Copy link
Author

@valeriosetti @tomi-font Sorry for being unclear, but I am currently working at IAR on IAR support in zephyr :)

This is not an issue in mbedtls, but only an issue with zephyr AND mbedtls, and I thought it would be easiest to fix it here.
As I said before this is only needed on old IAR and old IAR will not be able to build zephyr anyways. Also in many cases it looks completely unnecessary since __ARM_FEATURE_UNALIGNED is usually set.

Changing the __packed keyword everywhere is quite difficult task and we don't want to have our toolchain support PR depend on such a fix unless deemed absolutely necessary. (if you think otherwise I could make an attempt, but I would want help identifying all the potential modules and libraries where this needs to be replaced)

I tried lifting this issue in #toolchain on discord but I got no responses, I would be happy to discuss it there as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants